Skip to content

feat(xtest): compare up to four benchmark arms in one run - #621

Closed
dmihalcik-virtru wants to merge 2 commits into
mainfrom
DSPX-4372-02-karm-core
Closed

dmihalcik-virtru wants to merge 2 commits into
mainfrom
DSPX-4372-02-karm-core

Conversation

@dmihalcik-virtru

Copy link
Copy Markdown
Member

Split out of #583. Stacked on #620.

Generalizes the paired A/B benchmark from a fixed baseline/candidate pair to 2-4 named arms, so a bake-off between competing implementations of the same feature is one measurement on one runner instead of two separate two-arm dispatches whose ratios share no denominator.

  • perf/runner.py: Arm/CellResult keyed by arm_ids and an explicit reference, replacing the baseline/candidate pair. The A/A control now runs K copies of the reference (not a cheap pair), since the last arm in a K-arm round carries more within-round drift than an adjacent pair does.
  • perf/stats.py: every non-reference arm gets its own gated contrast against the reference; every pair of non-reference arms gets a symmetric FASTER/SLOWER/TIED head-to-head that never gates the build. Three BH-corrected families (gated / symmetric / ungated) so a bake-off costs the regression gate no power.
  • perf/report.py, fixtures/bench.py, conftest.py: --bench-refs (with --bench-baseline/--bench-candidate kept as two-arm shorthand), arm resolution and comparability checks generalized to K arms, and bake-off results rendered in the job summary and terminal output.
  • xtest.yml: bench-refs/bench-budget-seconds/bench-max-rounds inputs, a validate-and-normalize step, and a per-arm resolution step in the bench job. Budget defaults scale by arm-count/2 since a K-arm round costs K invocations, and the job timeout goes to 240m to cover it.

Depends on #620 (otdf-sdk-mgr slash-flattening): named-ref arms can be arbitrary branches, and an unflattened tag breaks build discovery.

A branch ref like 'feat/DSPX-2604-createtdf-chunked' resolved by name (not
by SHA) kept its slash in the tag, nesting dist/<tag>/ and src/<tag>/ one
level deeper than every consumer expects: xtest's all_versions_of() lists
dist/*/ and the Go Makefile finds src/*/, so the build was silently
discovered as a bare 'feat' directory with no cli.sh in it.

Flatten the tag the same way _classify_sha_match already flattens a branch
reached by SHA.
Generalizes the paired A/B benchmark from a fixed baseline/candidate pair to
2-4 named arms, so a bake-off between competing implementations of the same
feature is one measurement on one runner instead of two separate two-arm
dispatches whose ratios share no denominator.

- perf/runner.py: Arm/CellResult keyed by arm_ids and an explicit reference,
  replacing the baseline/candidate pair. The A/A control now runs K copies of
  the reference (not a cheap pair), since the last arm in a K-arm round
  carries more within-round drift than an adjacent pair does. Also folds in
  trunk's independent refinements: per-arm deadline capping so one slow arm
  can't run past the shared budget, and a strict configured-minimum-rounds
  check (previously only the statistical hard floor was enforced).
- perf/stats.py: every non-reference arm gets its own gated contrast against
  the reference; every pair of non-reference arms gets a symmetric
  FASTER/SLOWER/TIED head-to-head that never gates the build. Three
  BH-corrected families (gated / symmetric / ungated) so a bake-off costs the
  regression gate no power. Also folds in trunk's has_candidate_comparisons
  fix, which catches a control-only run that trunk's simpler nothing_measured
  check would have missed.
- perf/report.py, fixtures/bench.py, conftest.py: --bench-refs (with
  --bench-baseline/--bench-candidate kept as two-arm shorthand), arm
  resolution and comparability checks generalized to K arms, and bake-off
  results rendered in the job summary and terminal output.
- xtest.yml: bench-refs/bench-budget-seconds/bench-max-rounds inputs, a
  validate-and-normalize step, and a per-arm resolution step in the bench
  job. Budget defaults scale by arm-count/2 since a K-arm round costs K
  invocations, and the job timeout goes to 240m to cover it.

Depends on the otdf-sdk-mgr slash-flattening fix: named-ref arms can be
arbitrary branches, and an unflattened tag breaks build discovery.
@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

Base automatically changed from DSPX-4372-01-resolve-slash-flatten to main September 28, 2026 14:37
@dmihalcik-virtru

Copy link
Copy Markdown
Member Author

Superseded, not abandoned. This draft does six unrelated things in 3211 lines — fixes a statistical bug, converts two arms to K, adds head-to-head conclusions, rewrites the report, and reimplements benchmark input parsing in workflow shell — and is not reviewable as one unit.

Replaced by the six-stage stack described in spec/DSPX-4372-t1.md, starting with #625. Review found three defects here that the split fixes rather than carries forward: the control-key registration was moved after the RSS-censoring continue in runner.analyze (a censored control-only run then exits green claiming it measured something) and both tests that would have caught it were deleted; bake_offs names a winner from point estimates alone after reading one of three contrasts at four arms; and a tripped noise control does not suppress the winner sentence.

The DSPX-4372-02-karm-core branch is deliberately left in place — #622 and #623 are still based on it.

@dmihalcik-virtru

Copy link
Copy Markdown
Member Author

Stack progress: #625 (stage 1, statistical fixes, CI green) and #626 (stage 2, shared local benchmark preparation, stacked on #625) are open. Stages 3–6 — multi-arm measurement, pairwise conclusions, thin CI integration, report presentation — follow as their dependencies land.

One consequence worth stating plainly for #622 and #623: their base branch DSPX-4372-02-karm-core will now never merge, so they are dangling until someone retargets them.

dmihalcik-virtru added a commit that referenced this pull request Oct 8, 2026
## Note to the Reviewer

- This change separates the 'faster' and 'slower' outcomes for the
statistical significance test.
- Why?
- The benchmark blocked on [the Bejamini-Hochberg adjusted
p-values](https://en.wikipedia.org/wiki/False_discovery_rate#Benjamini%E2%80%93Hochberg_procedure),
which should be adjusted independently to take into account the skewed
distribution that will be present in most benchmarks.
- What?
- Review `stats.py` and `report.py` for the actual changes; which are
exercised in the tests.
   -  Other things of interest: 
      - The README in `perf` now has some fun charts
- The plan in the `spec` folder is just there to break down #621, itself
a small part in a larger PR I vibe coded a few months ago to improve the
benchmark workflow's functionality (to allow comparing multiple versions
of the same SDK) and readability/usefulness of the reports

Below is Claude's explanation, if you want more details.

## What this does

Fixes a real statistical defect on `main`, restores the regression
coverage
that guards it, and documents the resulting decision rule with three
generated
figures. Two-arm only — no multi-arm, no report changes, no CI changes.

**The IMPROVED verdict was decided with an invalid test.**
`_verdict_for`
concluded a candidate was faster when `p > 1 - alpha`, where `p` is the
*BH-adjusted upper-tail* (slower) p-value. Benjamini–Hochberg controls
small
p-values and generally pushes large ones toward 1, so that backwards
test gets
*easier* as a run grows rather than safer. There was no faster-tail
p-value
computed anywhere.

Now:

- `_one_sided_p` becomes `_one_sided_ps`, returning `(slower, faster)`
from
  `wilcoxon(alternative="greater")` and `wilcoxon(alternative="less")`.
- `PairedComparison` carries `p_value_faster` and `p_adjusted_faster`.
- Both tails are BH-adjusted independently within the same two existing
  correction families (gated, rest).
- IMPROVED becomes `ci_high < 1/threshold and p_faster < alpha` — the
mirror
  image of the REGRESSION clause, not a reversal of it.

## Restored coverage

`test_control_only_run_measured_nothing_about_the_candidate` and
`test_control_plus_candidate_counts_as_measured` are preserved here,
plus a new
case they did not cover: a control-only run whose RSS is pinned at the
measurement floor must still report `nothing_measured`. `analyze`
registers
control keys *before* the metric-specific censoring `continue` for
exactly this
reason; reorder those two statements and the new test fails, which is
the point.

## Documentation (second commit)

`perf/README.md` gains three figures explaining the decision rule, plus
the
machinery to keep them honest:

- `docs/svgkit.py` — just enough SVG to draw them. No tick algorithm, no
domain
inference, no text measurement; ticks are literals and gutters are
hand-laid.
Colors emit as CSS variables so one geometry pass serves light and dark.
- `docs/verify.py` — the Monte Carlo behind every number.
- `docs/make_figures.py` — renders from *frozen constants*, never from a
live
simulation, so a scipy release that shifts a tie correction cannot
silently
rewrite three committed SVGs. `--verify` re-runs the simulation and
reports
  mismatches; `--check` fails on figure drift.

The figures cover where the three verdicts sit on the ratio axis, the
multiplicity correction running backwards (the defect this PR fixes),
and what
the log transform does and does not fix about positive right-tailed
timings.

## Review corrections

Review flagged three claims in the docs as overstated. Each was
reproduced,
confirmed, and corrected — the fixes are in the figures and the
simulation, not
just the prose:

| Claim as written | What the simulation actually shows | Fix |
| --- | --- | --- |
| Figure 2 counted "false IMPROVED verdicts" | It counted only clause 2.
The CI clause passes **0 of 2000** null cells, so neither rule ever
completed a verdict on this null | Relabelled as p-clause acceptances;
`simulate_null_improvement_ci` added and pinned, so the masking is
stated rather than implied |
| "The CI clause already implies the unadjusted p-clause" | Not an
implication — 22 log-ratios just above the margin against 8 swinging far
below give **CI [1.211, 1.223] at p = 0.343** | Scoped to the harness's
own noise model; `ci_without_significance` pins the counterexample |
| Figure 3 called 0.362 an "actual size" | Stalling one arm moves the
true median off 1, so that is power against a 2.6% effect. Stall
**both** arms — equally heavy, but symmetric — and the slower tail holds
at **0.039–0.057** | "size" → "rejection rate" throughout;
`simulate_stall_size` added and plotted as a `both arms stalled` row |

The third correction strengthens the argument rather than weakening it:
signed-rank is not being invalidated by skew, it is correctly answering
a
point-null question that is the wrong question for a gate. That is
precisely
why the margin lives in the CI clause, which is the conjunction this PR
relies
on.

## Validation

- `uv run ruff check .`, `uv run ruff format .`, `uv run pyright` —
clean.
- 115 offline tests pass (was 106).
- `make_figures --verify` — every frozen constant matches a fresh
simulation.
- `make_figures --check` — committed SVGs match the generator byte for
byte.
- Mutation-verified: reverting the IMPROVED clause to `p_slower > 1 -
alpha`
fails `test_adjusted_slower_tail_is_not_read_as_evidence_of_improvement`
(adjusted p 0.99 on a case whose true faster-tail p is 0.30); inverting
the
censoring order fails
`test_censored_control_only_run_still_measured_nothing`.

## Compatibility

JSON output stays `"schema": 1`, with two added fields. No CLI flags
change.
The `docs/` package is documentation only — nothing in the harness
imports it.

## Dependencies

None — branches off `main`. First of six stages described in
`spec/DSPX-4372-t1.md` (added here), which together replace #621.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Features**
* Performance reports now include faster-tail p-values and their
separate multiple-comparison adjustments, while retaining the existing
report schema.
* Improvement and regression verdicts now use their respective
statistical tests alongside confidence-interval criteria; cases with
insufficient or invalid data remain inconclusive.
* **Documentation**
* Expanded guidance on directional tests, adjustment across comparisons,
verdict criteria, and how confidence intervals and p-values contribute.
* Added illustrative figures explaining verdict regions and statistical
behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant